Skip to content

api: Define OperatorsAuthentication model#950

Merged
mbarnes merged 1 commit intomainfrom
operators-authentication
Dec 10, 2024
Merged

api: Define OperatorsAuthentication model#950
mbarnes merged 1 commit intomainfrom
operators-authentication

Conversation

@mbarnes
Copy link
Collaborator

@mbarnes mbarnes commented Dec 9, 2024 •
edited
Loading

What this PR does

Adds PlatformProfile.operatorsAuthentication field to the Azure API, structured to resemble the azure.operators_authentication OCM schema (with minor tweaks to eliminate redundancy in field names, and simplifications where data is provided through ARM-defined models or headers)

Jira: ARO-10911 - Frontend API Changes to support managed identities
Link to demo recording:

Special notes for your reviewer

  • The main file to focus on is hcpCluster-models.tsp. The rest of the changes are generated from it.
  • There is no plumbing to Cluster Service yet. I want to get initial sign-off on the API changes first.
  • Visibility flags may need to be tweaked. Obviously the operator arrays should be replaceable for OpenShift versions beyond the initial install, but I'm not sure what other fields we should allow to be patched.

@mbarnes mbarnes mentioned this pull request Dec 9, 2024
Merged
bennerv
bennerv reviewed Dec 10, 2024
View reviewed changes
bennerv
bennerv reviewed Dec 10, 2024
View reviewed changes
bennerv
bennerv reviewed Dec 10, 2024
View reviewed changes
api/redhatopenshift/HcpCluster.Management/hcpCluster-models.tsp Outdated
/** The control plane operator name */
operatorName: string;

...UserAssignedManagedIdentity;
Copy link
Member

@bennerv bennerv Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the client-id and principal-id will be duplicated in both the identity{} field and this field if we choose to specify it here.

properties: {
    "managedIdentityProfile": {
        "controlPlaneOperators": [
             {
             "operatorName": "csi-disk-driver", 
             "resourceId": "/subscriptions/.../Microsoft.Identity/managedIdentity/csi-disk-driver"
             "principalId": "csi-disk-driver",
             "clientId": "csi-disk-driver"
             }, 
             {...}
         ]
    }   
},
identity: {
    "/subscriptions/.../Microsoft.Identity/managedIdentity/csi-disk-driver": {
        "principalId": "csi-disk-driver",
        "clientId": "csi-disk-driver"
    }, 
    "/subscriptions/.../csi-file-driver": {
        "principalId": "csi-file-driver",
        "clientId": "csi-file-driver"
    }
}

Copy link
Collaborator Author

@mbarnes mbarnes Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I had no idea what the top-level identity section would contain. In that case we can have both data and control plane operators be a map of operatorName -> resourceId where resourceId is a lookup key for the identity section (validation would fail if key is not found).

Copy link
Collaborator Author

@mbarnes mbarnes Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, maybe the serviceManagedIdentity field just needs to be a resource ID lookup key as well?

Copy link
Collaborator Author

@mbarnes mbarnes Dec 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to call out here, the RP does not currently parse the top-level identity section at all so we'll need to get that implemented immediately in addition to the plumbing needed for this new API.

@miguelsorianod
Copy link
Collaborator

miguelsorianod commented Dec 10, 2024

I see some of the attributes have removed parts of the names compared to in the CS API. In #858 (comment) I described and reasoned why in the CS API we intentionally kept parts of the names that initially might seem "redundant"

@mbarnes
Copy link
Collaborator Author

mbarnes commented Dec 10, 2024 •
edited
Loading

I described and reasoned why in the CS API we intentionally kept parts of the names that initially might seem "redundant"

I'm neutral on naming and will change it based on consensus, but FTR I felt the purpose of the operator maps were clear enough from the enclosing scopes.

properties: {
  platform: {
    operatorsAuthentication: {
      managedIdentities: {
        controlPlaneOperators: {...}
        dataPlaneOperators: {...}
      }
    }
  }
}

I'm considering renaming the managedIdentities section to userAssignedIdentities for additional clarity.

@mbarnes mbarnes mentioned this pull request Dec 10, 2024
Merged
@github-actions
Copy link

github-actions bot commented Dec 10, 2024

Please rebase pull request.

api: Define OperatorsAuthentication model
46acea7 
This is an extension to PlatformProfile that defines user-assigned
managed identities for individual OpenShift cluster operators.
@mbarnes mbarnes force-pushed the operators-authentication branch from 65cb766 to 46acea7 Compare December 10, 2024 16:36
@mbarnes mbarnes enabled auto-merge December 10, 2024 16:36
SudoBrendan
SudoBrendan approved these changes Dec 10, 2024
View reviewed changes
Copy link
Collaborator

@SudoBrendan SudoBrendan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is clear on the various kinds of auth required and what fields should be provided for each. We should probably get BU feedback on names just to be sure, unless that's tracked somewhere I'm unaware of?

@mbarnes mbarnes merged commit e4f974f into main Dec 10, 2024
@mbarnes mbarnes deleted the operators-authentication branch December 10, 2024 17:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bennerv bennerv bennerv left review comments

@miguelsorianod miguelsorianod miguelsorianod left review comments

@SudoBrendan SudoBrendan SudoBrendan approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mbarnes @miguelsorianod @bennerv @SudoBrendan

Comments